-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add FirewallRule class #86
Conversation
@@ -0,0 +1,59 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-07/schema#", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we need to port these changes to the AWS integration? It looks like we just put these rules in an array on a key for that integration.
https://bitbucket.org/jupiterone/jupiter-integration-aws/src/b1b082a76600f85738911adbea9d4112869059e7/src/integration-aws/ec2/converters/entities.ts?at=master#entities.ts-583
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also assume you already took a look at Rule, but thought I'd bring that up as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if this goes through I think it'd be good to get it into existing integrations like AWS, since the JSON.stringify(ipPermission)
operation makes these properties useless from a query language perspective.
I have looked at Rule
but IMO a firewall rule is a super-specific class of configuration (as indicated by the common properties shared across all firewall rules)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me so long as you have determined we need this. I would like a little documentation on the PR for easier debugging in the future before you merge this though.
src/schemas/FirewallRule.json
Outdated
"type": "integer" | ||
}, | ||
"source": { | ||
"description": "Source of the firewall rule. Can be IP address, CIDR range, 'Any', or other firewall-defined options.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we recommend only *
instead of Any
?
src/schemas/FirewallRule.json
Outdated
"format": "ip" | ||
}, | ||
"sourcePort": { | ||
"description": "Source port of the firewall rule. Typically an integer between 0 and 65535, but could also be 'Any', '*', or range of ports.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any
, *
, or "range of ports", will not be accepted when thetype
is integer
"format": "ip" | ||
}, | ||
"destinationPort": { | ||
"description": "Destination port of the firewall rule. Typically an integer between 0 and 65535, but could also be 'Any', '*', or range of ports.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we recommend only *
instead of Any
?
src/schemas/FirewallRule.json
Outdated
"protocol": { | ||
"description": "The protocol ", | ||
"type": "string", | ||
"examples": ["TCP", "UDP", "Any"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about recommending *
here?
I did not follow the change here closely. Why is this class needed? I think we have a Rule and RuleSet class already? |
With the AWS integration, details of each rule are captures as properties on the relationship between the entity and what is allowed to/from the entity. I'm concerned that adding this will unnecessarily add more entities, which is undesired from both our perspective (data volume) and customer's (cost). |
@erkangz yes, I see how this works in AWS. My concern with putting firewall rules on edges is that we cannot be as expressive on edges as we can on nodes (at least in J1QL). For example, there's not currently a way in J1QL to express direction of edges, representing ingress/egress, and edges do not allow array properties for IP addresses, ports, or protocols defined by these rules. Those are some of my main concerns when assigning firewall rules to edges. As for creating a new class |
Valid concern. However, that's a query lang issue/limitation and I don't think we should change the data-model because of that. For direction, specifically to firewall rules, we do have |
tldr; I would stick with the current approach until we can prove it won't work (perhaps examples from Azure or Google Cloud) and we should develop docs describing the firewall model and how to implement it. A few thoughts:
Here are some reasons I recall that relationships were chosen.
@erkangz, do you have others? |
Note that there is a TypeScript type for rule relationships: data-model/src/relationships.ts Line 1 in de4d2ff
|
No description provided.